Skip to content

C++: Support dynamic memory allocations in IR alias analysis#2797

Merged
jbj merged 11 commits into
github:masterfrom
rdmarsh2:rdmarsh/cpp/malloc-alias-locations
Mar 2, 2020
Merged

C++: Support dynamic memory allocations in IR alias analysis#2797
jbj merged 11 commits into
github:masterfrom
rdmarsh2:rdmarsh/cpp/malloc-alias-locations

Conversation

@rdmarsh2

@rdmarsh2 rdmarsh2 commented Feb 8, 2020

Copy link
Copy Markdown
Contributor

This adds a new opcode, InitializeDynamicAllocation, adds it to IR generation for statically-resolvable calls to AllocationFunctions, and adds a new Allocation subtype in the aliased_ssa version of AliasConfiguration.qll.

@rdmarsh2 rdmarsh2 added the C++ label Feb 8, 2020
@rdmarsh2 rdmarsh2 requested review from dbartol and jbj February 8, 2020 00:45

@rdmarsh2 rdmarsh2 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few TODOs added early that didn't seem to cause any issues, but I'd like a second look at them before I take them out

Comment thread cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCall.qll Outdated

@dbartol dbartol left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM. I'll do a final pass once you've taken it out of Draft status and the tests are passing.

Comment thread cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll Outdated
@rdmarsh2 rdmarsh2 marked this pull request as ready for review February 11, 2020 21:12
@rdmarsh2 rdmarsh2 requested review from a team as code owners February 11, 2020 21:12
@rdmarsh2 rdmarsh2 requested a review from dbartol February 11, 2020 21:12
Comment thread cpp/ql/src/semmle/code/cpp/security/TaintTracking.qll
@rdmarsh2 rdmarsh2 requested a review from geoffw0 February 12, 2020 23:23
dbartol
dbartol previously approved these changes Feb 13, 2020

@dbartol dbartol left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo typo

dbartol
dbartol previously approved these changes Feb 13, 2020

@dbartol dbartol left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dbartol dbartol left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rdmarsh2 We need perf numbers for this before we merge, but other than that it looks ready to go. I don't expect any negative perf impact.

@dbartol dbartol dismissed their stale review February 19, 2020 22:52

Waiting for perf numbers

@rdmarsh2 rdmarsh2 added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Feb 26, 2020
@rdmarsh2

Copy link
Copy Markdown
Contributor Author

https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/869/ finished - the analysis with this change was about 5% faster on Wireshark and ChakraCore. That might be noise or it might be from having fewer references to all aliased memory in SSA construction.

@jbj jbj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The internal tests have passed, so I'll merge this now.

@jbj jbj merged commit ec85f9f into github:master Mar 2, 2020
@rdmarsh2 rdmarsh2 deleted the rdmarsh/cpp/malloc-alias-locations branch March 2, 2020 18:05
@jbj jbj mentioned this pull request Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants